fix: 독서 기록 pagenumber로 order by 시 페이징처리 동일한 id가 내려오는 상황 #97
Conversation
Walkthrough페이지 번호 정렬 시 안정성을 확보하기 위해 단일 OrderSpecifier 반환을 배열(Array<OrderSpecifier<*>>)로 변경하고, 페이지 번호 정렬일 때 updatedAt desc를 보조 정렬로 추가했다. 메서드명을 createOrderSpecifier → createOrderSpecifiers로 변경했으며, 페이징 및 총계 계산 로직은 변경 없다. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Repository
participant QueryDSL
participant DB
Caller->>Repository: findReadingRecords(sort, page)
Repository->>QueryDSL: build query with orderBy(pageNumber, updatedAt desc) or createdAt
QueryDSL->>DB: SELECT ... ORDER BY ...
DB-->>QueryDSL: rows
QueryDSL-->>Repository: results
Repository-->>Caller: paged results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesTip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt(2 hunks)
🧰 Additional context used
🪛 detekt (1.23.8)
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
[warning] 34-34: In most cases using a spread operator causes a full copy of the array to be created before calling a method. This may result in a performance penalty.
(detekt.performance.SpreadOperator)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-validation
🔇 Additional comments (1)
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt (1)
34-34: 정렬 대상 컬럼은 모두 non-null 제약이 있습니다
- infra/src/main/kotlin/org/yapp/infra/readingrecord/entity/ReadingRecordEntity.kt
·@Column(name = "page_number", nullable = false)- infra/src/main/kotlin/org/yapp/infra/common/BaseTimeEntity.kt
·@Column(name = "created_at", nullable = false)
·@Column(name = "updated_at", nullable = false)따라서 DB에서 해당 칼럼이
NULL이 될 가능성이 없으므로 별도의nullsLast()/nullsFirst()지정은 필요하지 않습니다.
| .selectFrom(readingRecord) | ||
| .where(whereCondition) | ||
| .orderBy(createOrderSpecifier(sort)) | ||
| .orderBy(*createOrderSpecifiers(sort)) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
스프레드 연산자 사용(detekt 경고) — 영향 미미하나 억제 또는 구조 변경을 고려하세요
.orderBy(*createOrderSpecifiers(sort))는 detekt의 SpreadOperator 경고 대상입니다. 현재 배열 길이가 1~2 수준이라 성능 영향은 사실상 미미합니다. 유지보수를 위해 그대로 두되 경고 억제 또는 호출부 구조 변경 중 하나를 권장합니다.
대안 1: 함수 레벨에서 경고 억제
@Suppress("SpreadOperator")
override fun findReadingRecordsByDynamicCondition(
userBookId: UUID,
sort: ReadingRecordSortType?,
pageable: Pageable
): Page<ReadingRecordEntity> {
// ...
}대안 2: 스프레드 없이 체이닝
createOrderSpecifiers를 사용하지 않고when분기 내에서.orderBy(a, b)형태로 직접 지정- 변경폭이 커지므로 본 PR 범위를 넘는다면 대안 1이 현실적입니다.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 34-34: In most cases using a spread operator causes a full copy of the array to be created before calling a method. This may result in a performance penalty.
(detekt.performance.SpreadOperator)
🤖 Prompt for AI Agents
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
around line 34: Detekt flags the spread operator in
`.orderBy(*createOrderSpecifiers(sort))`; to address this either suppress the
warning on the function with @Suppress("SpreadOperator") or remove the spread by
inlining orderBy arguments (use explicit chaining or branch on sort and call
.orderBy(a, b) directly); pick suppression if you want minimal changes, or
refactor the ordering logic to avoid creating and spreading an array if you
prefer to eliminate the warning fully.
| private fun createOrderSpecifiers(sort: ReadingRecordSortType?): Array<OrderSpecifier<*>> { | ||
| return when (sort) { | ||
| ReadingRecordSortType.PAGE_NUMBER_ASC -> readingRecord.pageNumber.asc() | ||
| ReadingRecordSortType.PAGE_NUMBER_DESC -> readingRecord.pageNumber.desc() | ||
| ReadingRecordSortType.CREATED_DATE_ASC -> readingRecord.createdAt.asc() | ||
| ReadingRecordSortType.CREATED_DATE_DESC -> readingRecord.createdAt.desc() | ||
| null -> readingRecord.createdAt.desc() | ||
| ReadingRecordSortType.PAGE_NUMBER_ASC -> arrayOf( | ||
| readingRecord.pageNumber.asc(), | ||
| readingRecord.updatedAt.desc() | ||
| ) | ||
|
|
||
| ReadingRecordSortType.PAGE_NUMBER_DESC -> arrayOf( | ||
| readingRecord.pageNumber.desc(), | ||
| readingRecord.updatedAt.desc() | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
타이브레이커 부족 — id로 최종 정렬 기준을 추가해 페이징 안정성을 보장하세요
updatedAt.desc()를 보조 정렬로 추가한 것은 합리적입니다. 다만 동일 pageNumber와 동일 updatedAt이 동시에 존재할 경우(타임스탬프 해상도/동시 업데이트 등) 여전히 정렬이 비결정적이어서 오프셋 페이징에서 중복/누락이 재현될 수 있습니다. 최종 타이브레이커로 유일 키(일반적으로 id)를 추가해 결정적 정렬을 보장하세요.
권장 변경(diff):
ReadingRecordSortType.PAGE_NUMBER_ASC -> arrayOf(
readingRecord.pageNumber.asc(),
- readingRecord.updatedAt.desc()
+ readingRecord.updatedAt.desc(),
+ readingRecord.id.desc()
)
ReadingRecordSortType.PAGE_NUMBER_DESC -> arrayOf(
readingRecord.pageNumber.desc(),
- readingRecord.updatedAt.desc()
+ readingRecord.updatedAt.desc(),
+ readingRecord.id.desc()
)- updatedAt vs createdAt에 대한 질문에 답변: “페이지 번호 내에서 최근에 수정된 기록을 우선”이라는 제품 의도가 있다면
updatedAt.desc()가createdAt보다 더 적절합니다. 단, “등록 순서” 의미가 더 중요하다면createdAt을 택할 수 있습니다. 어떤 의미가 UX/도메인에 맞는지 팀 합의가 필요합니다.
성능(인덱스) 관점:
- 현재 where:
(userBookId, deletedAt IS NULL)+ order:(pageNumber [asc/desc], updatedAt desc, id desc)조합을 자주 사용한다면 해당 컬럼 순서로의 복합 인덱스 추가를 고려하세요.- 예:
(user_book_id, deleted_at, page_number, updated_at, id)
- 예:
- createdAt 정렬 경로도 빈번하다면
(user_book_id, deleted_at, created_at, id)인덱스가 유효합니다.
오프셋 페이징의 본질적 한계:
- updatedAt이 자주 변하는 도메인이라면 페이지를 넘기는 사이에 레코드가 이동하여 중복/누락이 발생할 수 있습니다. 장기적으로는 keyset 페이징(커서 기반) 전환을 검토하세요.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun createOrderSpecifiers(sort: ReadingRecordSortType?): Array<OrderSpecifier<*>> { | |
| return when (sort) { | |
| ReadingRecordSortType.PAGE_NUMBER_ASC -> readingRecord.pageNumber.asc() | |
| ReadingRecordSortType.PAGE_NUMBER_DESC -> readingRecord.pageNumber.desc() | |
| ReadingRecordSortType.CREATED_DATE_ASC -> readingRecord.createdAt.asc() | |
| ReadingRecordSortType.CREATED_DATE_DESC -> readingRecord.createdAt.desc() | |
| null -> readingRecord.createdAt.desc() | |
| ReadingRecordSortType.PAGE_NUMBER_ASC -> arrayOf( | |
| readingRecord.pageNumber.asc(), | |
| readingRecord.updatedAt.desc() | |
| ) | |
| ReadingRecordSortType.PAGE_NUMBER_DESC -> arrayOf( | |
| readingRecord.pageNumber.desc(), | |
| readingRecord.updatedAt.desc() | |
| ) | |
| private fun createOrderSpecifiers(sort: ReadingRecordSortType?): Array<OrderSpecifier<*>> { | |
| return when (sort) { | |
| ReadingRecordSortType.PAGE_NUMBER_ASC -> arrayOf( | |
| readingRecord.pageNumber.asc(), | |
| readingRecord.updatedAt.desc(), | |
| readingRecord.id.desc() | |
| ) | |
| ReadingRecordSortType.PAGE_NUMBER_DESC -> arrayOf( | |
| readingRecord.pageNumber.desc(), | |
| readingRecord.updatedAt.desc(), | |
| readingRecord.id.desc() | |
| ) |
🤖 Prompt for AI Agents
In
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
around lines 48–59, the order specifiers lack a final deterministic tiebreaker
which can cause nondeterministic paging when pageNumber and updatedAt are equal;
add the primary key (id) as the last OrderSpecifier (descending or ascending
consistent with desired newest-first/oldest-first tie-breaking) to both
PAGE_NUMBER_ASC and PAGE_NUMBER_DESC branches so sorting is fully deterministic;
optionally confirm whether updatedAt or createdAt is the intended second-level
sort and, if this sort pattern is frequent, consider adding a composite DB index
(e.g., user_book_id, deleted_at, page_number, updated_at, id) to support the new
ordering.
| ReadingRecordSortType.PAGE_NUMBER_ASC -> arrayOf( | ||
| readingRecord.pageNumber.asc(), | ||
| readingRecord.updatedAt.desc() | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
updatedAt을 보조 정렬로 사용한 선택은 타당합니다
동일 pageNumber 내에서 “최근 수정 기준”을 기대한다면 updatedAt.desc()는 도메인 의도에 부합합니다. 위 코멘트대로 최종 타이브레이커(id)만 추가해 주시면 더 이상 중복 위험이 없습니다.
🤖 Prompt for AI Agents
In
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
around lines 50 to 53, the sort array uses pageNumber and updatedAt but lacks a
final tiebreaker; append the entity id comparator as the last sort key (e.g.,
readingRecord.id.asc() or .desc() depending on desired stable order) so results
are deterministic and duplicate-key risks are eliminated.
| ReadingRecordSortType.CREATED_DATE_ASC -> arrayOf(readingRecord.createdAt.asc()) | ||
| ReadingRecordSortType.CREATED_DATE_DESC -> arrayOf(readingRecord.createdAt.desc()) | ||
| null -> arrayOf(readingRecord.createdAt.desc()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
createdAt 정렬에도 유일 타이브레이커(id) 추가를 고려
본 PR 범위를 넘는 제안이지만, CREATED_DATE_ASC/DESC에서도 동일 타임스탬프가 가능하므로 최종적으로 id를 추가하면 전역적으로 페이징 안정성이 향상됩니다.
권장 변경(diff):
- ReadingRecordSortType.CREATED_DATE_ASC -> arrayOf(readingRecord.createdAt.asc())
- ReadingRecordSortType.CREATED_DATE_DESC -> arrayOf(readingRecord.createdAt.desc())
- null -> arrayOf(readingRecord.createdAt.desc())
+ ReadingRecordSortType.CREATED_DATE_ASC -> arrayOf(
+ readingRecord.createdAt.asc(),
+ readingRecord.id.desc()
+ )
+ ReadingRecordSortType.CREATED_DATE_DESC -> arrayOf(
+ readingRecord.createdAt.desc(),
+ readingRecord.id.desc()
+ )
+ null -> arrayOf(
+ readingRecord.createdAt.desc(),
+ readingRecord.id.desc()
+ )단위 테스트 보강 제안:
- 동일 createdAt/updatedAt를 가진 샘플 데이터로 오프셋 페이징 시 중복/누락이 없는지 검증하는 테스트를 추가하면 회귀 방지에 도움이 됩니다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ReadingRecordSortType.CREATED_DATE_ASC -> arrayOf(readingRecord.createdAt.asc()) | |
| ReadingRecordSortType.CREATED_DATE_DESC -> arrayOf(readingRecord.createdAt.desc()) | |
| null -> arrayOf(readingRecord.createdAt.desc()) | |
| } | |
| ReadingRecordSortType.CREATED_DATE_ASC -> arrayOf( | |
| readingRecord.createdAt.asc(), | |
| readingRecord.id.desc() | |
| ) | |
| ReadingRecordSortType.CREATED_DATE_DESC -> arrayOf( | |
| readingRecord.createdAt.desc(), | |
| readingRecord.id.desc() | |
| ) | |
| null -> arrayOf( | |
| readingRecord.createdAt.desc(), | |
| readingRecord.id.desc() | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
infra/src/main/kotlin/org/yapp/infra/readingrecord/repository/impl/JpaReadingRecordQuerydslRepositoryImpl.kt
around lines 60 to 63, the sorting for CREATED_DATE_ASC/DESC currently only uses
createdAt which can produce non-deterministic ordering when timestamps are
identical; update the sort arrays to add id as a deterministic tie-breaker (for
CREATED_DATE_ASC use createdAt.asc(), id.asc(); for CREATED_DATE_DESC use
createdAt.desc(), id.desc(); keep the null default as createdAt.desc(),
id.desc()). Also add unit tests that create multiple records with identical
createdAt/updatedAt values and assert offset-based paging returns stable,
non-duplicated/omitted results.
move-hoon
changed the title
🔗 관련 이슈
📘 작업 유형
📙 작업 내역
JpaReadingRecordQuerydslRepositoryImpl의 정렬 조건 수정PAGE_NUMBER_ASC / DESC정렬 시 동일pageNumber를 가진 데이터가 섞이지 않도록updatedAt.desc()보조 정렬 조건 추가CREATED_DATE_ASC / DESC, 기본 정렬)은 기존 로직 유지🧪 테스트 내역
PAGE_NUMBER_ASC정렬 시 동일 페이지 번호 내 최신 수정 순서대로 정렬됨 확인PAGE_NUMBER_DESC정렬 시 동일 페이지 번호 내 최신 수정 순서대로 정렬됨 확인CREATED_DATE기준 정렬 영향 없음💬 리뷰 포인트
updatedAt으로 두는 게 적절한지, 혹은createdAt이나 다른 기준이 더 나을지 확인 부탁드립니다.Summary by CodeRabbit